• X
• Mastodon
• LinkedIn
• Facebook

tests/routers/openml/setups_untag_test.py (2)

25-45: Consider extracting shared arrange/assert DB helpers to reduce repetition.

setup_tag insert + post-condition deletion checks are repeated across multiple tests; a tiny helper would keep these tests shorter and easier to evolve.

Also applies to: 73-77, 97-116

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/routers/openml/setups_untag_test.py` around lines 25 - 45, Extract
repeated DB arrange/assert into small helper functions to reduce duplication:
create helpers like insert_setup_tag(expdb_test, setup_id, tag, uploader=2) that
wraps the expdb_test.execute(text("INSERT INTO setup_tag ..."), parameters=...)
and assert_setup_tag_absent(expdb_test, setup_id, tag) that runs the SELECT used
in the post-condition and asserts rows.all() is empty; then update tests that
call expdb_test.execute and the SELECT assert (and the py_api.post usage with
ApiKey.SOME_USER) to use these helpers (or place them in conftest or the test
module) to keep tests shorter and consistent.

---

50-58: Keep one API-level negative-path smoke test to protect HTTP contract.

These direct-call tests validate domain logic well, but they no longer assert router exception mapping (status/code/detail) for negative paths. Consider retaining one lightweight API failure case (e.g., tag not found) so endpoint contract regressions are caught.

Suggested addition

+@pytest.mark.mut
+async def test_setup_untag_api_tag_not_found(py_api: httpx.AsyncClient) -> None:
+    missing_tag = "missing_tag_for_api_contract"
+    response = await py_api.post(
+        f"/setup/untag?api_key={ApiKey.SOME_USER}",
+        json={"setup_id": 1, "tag": missing_tag},
+    )
+    # Assert endpoint-level error contract (status + structured payload fields)
+    assert response.status_code != HTTPStatus.OK
+    assert "code" in response.json()
+    assert "detail" in response.json()

Also applies to: 60-68, 72-87

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/routers/openml/setups_untag_test.py` around lines 50 - 58, Add one
API-level negative-path test alongside the direct domain test: create a new test
(e.g., test_api_setup_untag_unknown_setup) that uses the HTTP test client
fixture to call the router endpoint that maps to untag_setup (simulate removing
tag from setup_id=999999/tag="test_tag"), then assert the HTTP response maps the
domain SetupNotFoundError to the expected status/code/detail (e.g., 404 and the
"Setup <id> not found." detail) to protect the endpoint contract.

tests/routers/openml/setups_untag_test.py (1)

71-92: Good test for the authorization error path.

The test correctly verifies:

1. The exception is raised when a non-owner attempts removal
2. The tag remains in the database after the failed operation

Minor observation: using OWNER_USER for a test where the user does not own the tag may be confusing to future readers. If this naming follows existing project conventions, consider adding a brief comment clarifying the relationship (e.g., # OWNER_USER does not own this tag (uploader=2)).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/routers/openml/setups_untag_test.py` around lines 71 - 92, The test
test_setup_untag_not_owned_by_you uses OWNER_USER while inserting the setup_tag
row with uploader=2 which can be confusing; update the test to clarify that
OWNER_USER does not own the tag by either using a different user constant or
adding a one-line comment above the INSERT explaining "OWNER_USER does not own
this tag (uploader=2)", referencing the test function name
test_setup_untag_not_owned_by_you and the untag_setup call so reviewers see the
intent and the setup_tag insert parameters clearly.

tests/routers/openml/setups_untag_test.py (1)

49-61: Anchor the exception-message matches.

Line 50 currently accepts any numeric setup id, and the three match= patterns are all unanchored. Since these tests pass fixed inputs, tightening the patterns will catch message regressions more reliably.

♻️ Suggested diff

-    with pytest.raises(SetupNotFoundError, match=r"Setup \d+ not found."):
+    with pytest.raises(SetupNotFoundError, match=r"^Setup 999999 not found\.$"):
         await untag_setup(
             setup_id=999999,
             tag="test_tag",
             user=SOME_USER,
             expdb_db=expdb_test,
         )
@@
-    with pytest.raises(TagNotFoundError, match=rf"Setup 1 does not have tag '{tag}'\."):
+    with pytest.raises(TagNotFoundError, match=rf"^Setup 1 does not have tag '{tag}'\.$"):
         await untag_setup(
             setup_id=1,
             tag=tag,
             user=SOME_USER,
             expdb_db=expdb_test,
         )
@@
-        match=rf"You may not remove tag '{tag}' of setup 1 because it was not created by you\.",
+        match=rf"^You may not remove tag '{tag}' of setup 1 because it was not created by you\.$",
     ):
         await untag_setup(
             setup_id=1,
             tag=tag,
             user=OWNER_USER,

Also applies to: 77-80

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/routers/openml/setups_untag_test.py` around lines 49 - 61, Update the
regex assertions in tests to anchor the expected exception messages so they
match exactly; in test_setup_untag_unknown_setup change the match for
SetupNotFoundError to anchor the numeric id pattern (e.g. use ^Setup \d+ not
found\.$ or \ASetup \d+ not found\.\Z) and in test_setup_untag_tag_not_found
anchor the TagNotFoundError pattern to exactly match "Setup 1 does not have tag
'non_existent_tag_12345'." (e.g. ^Setup 1 does not have tag
'non_existent_tag_12345'\.$), and apply the same anchoring fix to the similar
assertions around lines 77-80; locate these in the tests invoking untag_setup
and the SetupNotFoundError/TagNotFoundError assertions.